Summary of Changes

Hello @martin-augment, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the floor function's optimization capabilities by introducing support for Decimal data types in its preimage calculation. This change allows the query optimizer to convert floor(decimal_column) = N conditions into more performant range filters, such as decimal_column >= N AND decimal_column < N + 1. The implementation includes a new utility function for decimal bound computation and is thoroughly validated with both unit tests and SQL logic tests to ensure correctness and proper query plan generation.

Highlights

• Decimal Support for Floor Preimage: Extended the floor function's preimage optimization to support all Decimal data types (Decimal32, Decimal64, Decimal128, Decimal256). This allows predicates like floor(decimal_col) = N to be transformed into efficient range scans (decimal_col >= N AND decimal_col < N + 1).
• New decimal_preimage_bounds Function: Introduced a new helper function decimal_preimage_bounds to calculate the lower and upper bounds for the floor preimage of decimal values, handling fractional parts (returning None) and potential overflow during bound calculation.
• Comprehensive Unit Tests: Added extensive unit tests for the new decimal preimage logic, covering valid integer-like decimal inputs, non-integer decimals, overflow scenarios, edge cases (min/max values), and null inputs across all Decimal types.
• SQL Logic Test Verification: A new SQL logic test file (floor_preimage.slt) was added to verify the optimization in the query planner for Decimal128, alongside existing Float64 and Int32 tests. This includes EXPLAIN plans to confirm the predicate transformation for various comparison operators.

Walkthrough

This change adds support for decimal types (Decimal32, Decimal64, Decimal128, Decimal256) to the floor preimage optimization logic in DataFusion. A new helper function decimal_preimage_bounds computes preimage ranges for decimal values with proper handling of precision, scale, and overflow. The core floor matching logic is extended to handle decimal scalar values. Additionally, a new sqllogictest file verifies the floor preimage transformation works correctly across different data types, comparison operators, and edge cases, including NULL handling and non-optimizable predicates.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review: Add Decimal support for floor preimage

Summary

This PR adds decimal type support (Decimal32, Decimal64, Decimal128, Decimal256) to the floor function's preimage optimization. The implementation looks solid overall with comprehensive test coverage. Here are my findings:

✅ Strengths

1. Comprehensive Test Coverage: Excellent unit tests covering all decimal types with edge cases (overflow, null, non-integer, min/max values)
2. Consistent Pattern: The implementation follows the existing pattern for float and integer types
3. Good Documentation: The SQL logic tests are well-documented and cover various comparison operators
4. Correctness: The decimal preimage logic correctly implements the mathematical constraint that floor always returns integers

🔍 Code Quality & Best Practices

1. Import Organization (datafusion/functions/src/math/floor.rs:22-27)

The imports are clean, but consider alphabetical ordering within groups:

use arrow::compute::{DecimalCast, rescale_decimal};
use arrow::datatypes::{
    ArrowNativeTypeOp, DataType, Decimal32Type, Decimal64Type, Decimal128Type,
    Decimal256Type, DecimalType, Float32Type, Float64Type,
};

2. Potential for Code Deduplication (datafusion/functions/src/math/floor.rs:262-301)

The four decimal type cases are nearly identical. While the current approach is clear and explicit, consider if a macro or generic helper could reduce repetition. However, the current explicit approach is acceptable for maintainability.

🐛 Potential Issues

1. Fractional Part Check Logic (datafusion/functions/src/math/floor.rs:378-379)

if scale > 0 && value % one_scaled != D::Native::ZERO {
    return None;
}

Concern: This check only applies when scale > 0, but what about when scale < 0? According to SQL standards, decimal types can have negative scales (e.g., DECIMAL(10, -2) means rounding to hundreds).

Questions:

• Does Arrow support negative scales for decimals?
• If so, should the fractional check be skipped or handled differently?
• The test cases only use scale >= 0, so this edge case is untested

Recommendation: Either:

• Add a comment explaining that negative scales are not supported/expected
• Add validation or handling for negative scales
• Add test coverage for scale = 0 and scale < 0 cases

2. Error Handling in rescale_decimal (datafusion/functions/src/math/floor.rs:368-373)

let one_scaled: D::Native = rescale_decimal::<D, D>(
    D::Native::ONE,
    1,
    0,
    precision,
    scale,
)?;

Observation: rescale_decimal returns Option, which is propagated with ?. This is correct, but it's worth documenting when this can fail:

• Invalid precision/scale combinations
• Overflow during scaling

Recommendation: Add a comment explaining failure conditions for future maintainers.

⚡ Performance Considerations

1. Modulo Operation: The fractional part check uses modulo (value % one_scaled), which is generally efficient for integer types. No concerns here.

2. rescale_decimal Call: This is called once per preimage computation, which is acceptable since this is an optimization path that runs during query planning, not query execution.

🔒 Security Concerns

No security issues identified. The implementation:

• Uses checked arithmetic (add_checked) to prevent overflow
• Properly handles null values
• Returns None for invalid inputs rather than panicking

🧪 Test Coverage

Excellent coverage with the following test categories:

• ✅ Valid cases for all decimal types
• ✅ Non-integer values (should return None)
• ✅ Overflow cases
• ✅ Edge cases (min/max values)
• ✅ Null handling
• ✅ All comparison operators (=, <>, <, >, <=, >=)
• ✅ IS DISTINCT FROM / IS NOT DISTINCT FROM

Potential additions:

1. Test with scale = 0 explicitly (currently in edge cases but worth a dedicated test)
2. Test with very high scales (e.g., scale = 38 for Decimal128)
3. Integration test showing the optimization actually improves query performance

📝 Documentation & Comments

Good:

• Function documentation is clear
• Test comments explain the mathematical constraints

Could improve:

• Add inline comment explaining why fractional values return None (datafusion/functions/src/math/floor.rs:378)
• Document the rescale_decimal approach in the function comment

🎯 Specific Recommendations

1. Address the scale < 0 edge case or add documentation

2. Consider adding a comment in decimal_preimage_bounds explaining:

   /// Compute preimage bounds for floor function on decimal types.
   /// For floor(x) = n, the preimage is [n, n+1).
   /// 
   /// Returns None if:
   /// - The value has a fractional part (floor always returns integers)
   /// - Adding 1 would overflow
   /// - rescale_decimal fails (invalid precision/scale)
   

3. Minor: Test naming consistency - some tests use decimal prefix, others don't. Consider consistent naming like:

   • test_floor_preimage_decimal32_*
   • test_floor_preimage_decimal64_*
     etc.

✅ Final Verdict

This is a high-quality PR that correctly implements decimal support for floor preimage optimization. The code is well-tested, follows existing patterns, and handles edge cases properly.

Recommended Action: Approve with minor suggestions for documentation improvements and clarification on negative scale handling.

Nice work! 🎉

Minor Nits

• Line 262-301: Consider adding blank lines between different decimal type cases for readability
• Test file header (datafusion/sqllogictest/test_files/floor_preimage.slt:18-25): Well-structured and informative

1. Fractional Part Check Logic (datafusion/functions/src/math/floor.rs:378-379)

if scale > 0 && value % one_scaled != D::Native::ZERO {
    return None;
}

Concern: This check only applies when scale > 0, but what about when scale < 0? According to SQL standards, decimal types can have negative scales (e.g., DECIMAL(10, -2) means rounding to hundreds).

Questions:

• Does Arrow support negative scales for decimals?
• If so, should the fractional check be skipped or handled differently?
• The test cases only use scale >= 0, so this edge case is untested

Recommendation: Either:

• Add a comment explaining that negative scales are not supported/expected
• Add validation or handling for negative scales
• Add test coverage for scale = 0 and scale < 0 cases

value:useful; category:bug; feedback: The Claude AI reviewer is correct! Apache Arrow supports negative scales for decimals, so this check won't detect such decimals with fractional part. Prevents a bug with decimals with negative scales. Also shows that there are no unit/slt tests for such.